feat: simulation foundation — models, ABC, factory, model registry, assets by cagataycali · Pull Request #84 · strands-labs/robots

cagataycali · 2026-04-01T01:01:36Z

TL;DR

Simulation foundation layer — dataclasses, SimEngine ABC, pluggable backend factory, URDF/MJCF model registry, asset manager with auto-download, user robot registration, and @tool for asset downloads. Expands robot registry from 38 → 68 robots. Pure Python, no MuJoCo dependency.

Rebased on main (post-#83). All 50 review threads resolved. All CI green.

What changed

Simulation abstractions (`strands_robots/simulation/`)

File	What
`models.py`	Dataclasses: `SimWorld`, `SimRobot`, `SimObject`, `SimCamera`, `TrajectoryStep`, `SimStatus`. Backend-agnostic — engine state stored in generic `_backend_state: dict` (no MuJoCo-specific fields).
`base.py`	`SimEngine` ABC — 12 required abstract methods + 4 optional (raise `NotImplementedError`). Context manager protocol. `__del__` logs cleanup errors instead of silencing.
`factory.py`	`create_simulation()` + `register_backend()` with duplicate protection (`ValueError` on conflict), built-in alias protection, `force=True` for intentional overwrites.
`model_registry.py`	URDF/MJCF resolution: user-registered → `STRANDS_ASSETS_DIR` → `~/.strands_robots/assets/` → CWD → `robot_descriptions` fallback. Logs asset manager availability.
`__init__.py`	Thin re-exports + `__getattr__` lazy loading. No MuJoCo references.

Assets (`strands_robots/assets/`)

File	What
`__init__.py`	Thin exports only (consistent with repo convention).
`manager.py`	Asset path resolution with `_safe_join()` path-traversal protection, `_resolve_candidates()` helper. Search paths documented. No bundled assets — everything from `robot_descriptions`.
`download.py`	All download logic lives here (421 lines): `robot_descriptions` → git clone fallback. `_shallow_clone()` validates URLs via `_ALLOWED_CLONE_URL_RE` (HTTPS github.com only).

Tools (`strands_robots/tools/`)

File	What
`download_assets.py`	Thin `@tool` wrapper (~78 lines) — delegates to `assets.download`. No duplicated logic.

Registry (`strands_robots/registry/`)

File	What
`user_registry.py`	`register_robot()` / `unregister_robot()` — persisted to `~/.strands_robots/user_robots.json`. Security warning on docstring re: MJCF code execution risk.
`loader.py`	Merges user-local registry on top of package `robots.json`. Public `invalidate_cache()` API (no private imports).
`robots.json`	38 → 68 robots (aerial, expressive, mobile_manip categories added).
`__init__.py`	Re-exports `register_robot`, `unregister_robot`, `list_user_robots`, `invalidate_cache`.

Other

File	What
`utils.py`	`get_base_dir()`, `get_assets_dir()`, `resolve_asset_path()` — single source of truth.
`README.md`	Environment variables table + cache directory docs.
`pyproject.toml`	`[sim]` extra with `robot_descriptions` dependency.

Design decisions

SimEngine ABC

from strands_robots.simulation.base import SimEngine
from strands_robots.simulation import create_simulation, register_backend

# 12 required methods — every physics engine must implement:
# create_world, destroy, reset, step, get_state, add_robot, remove_robot,
# add_object, remove_object, get_observation, send_action, render

# 4 optional methods (raise NotImplementedError):
# load_scene, run_policy, randomize, get_contacts

# get_observation/send_action are facade methods that bridge
# Sim ↔ Policy domains — the agent tool sees a single "simulation"
# interface without needing to know about Robot vs Sim distinction.

# Pluggable backends with alias support:
register_backend("bullet", lambda: BulletSim, aliases=["pb"])
sim = create_simulation("bullet")

Asset resolution order

1. STRANDS_ASSETS_DIR (env override)
2. ~/.strands_robots/assets/ (user cache)
3. CWD/assets/ (project-local)
4. robot_descriptions package (auto-download fallback)

Customer assets always win over defaults. Single env var: STRANDS_ASSETS_DIR.

Security

Path traversal: _safe_join() in both manager.py and download.py
URL validation: _shallow_clone() rejects non-HTTPS/non-github.com URLs
register_robot(): Library-only function, not exposed as @tool. Docstring warns about MJCF code execution risk if ever surfaced to agents.

Testing

232 tests pass, 6 skipped, 0 failures (0.98s)
- 14 simulation foundation tests (ABC contracts, factory round-trip, model registry, context manager)
- 28 user registry tests (register/unregister, persistence, validation, path traversal)
ruff check: All passed
ruff format: 55 files clean
mypy: 0 issues in 36 source files

Review history

Reviewer	Status	Threads
@yinsong1986	✅ APPROVED	3/3 resolved
@awsarron	CHANGES_REQUESTED → all addressed	30/30 resolved
@max-rattray-aws	COMMENTED → all addressed	3/3 resolved

Key changes since CHANGES_REQUESTED:

Rebased onto main (post-chore: modernize build system — uv lockfile, Python >=3.12 #83, Python ≥3.12 + uv)
Renamed SimulationBackend → SimEngine
Moved download logic from tools/ → assets/download.py (tools as thin wrappers)
Removed MuJoCo-specific fields from SimWorld → generic _backend_state
Added path-traversal protection + URL validation
Culled 10 superfluous tests
Security warning on register_robot() docstring

Unblocks #85 (MuJoCo backend) and #86 (Robot factory).

yinsong1986

All review comments addressed. LGTM.

cagataycali · 2026-04-06T07:00:05Z

Fixed in ef75a5d — added the missing [sim] extra to pyproject.toml:

sim = [
    "robot_descriptions>=1.11.0,<2.0.0",
]
all = [
    "strands-robots[groot-service]",
    "strands-robots[lerobot]",
    "strands-robots[sim]",
]

The code in assets/manager.py and tools/download_assets.py references robot_descriptions for asset downloads, and the docstrings say pip install strands-robots[sim], but the extra was never declared.

awsarron

Looking good, few small final comments.

Need to rebase after #83 is merged.

For all comments in this PR, we should examine common themes and include corrections for them in AGENTS.md so that future agent runs benefit from their lessons.

@awsarron

Address @awsarron review thread on manager.py — replace function-level lazy import of download.auto_download_robot with a module-level try/except that sets a sentinel (None) when the optional dependency is unavailable. This eliminates the circular-import design smell while preserving the same behavior: manager.py remains importable without robot_descriptions installed, and _auto_download_robot returns False immediately when the module is absent. Also fixes docstring reference: _safe_join → safe_join (canonical name).

cagataycali · 2026-04-22T00:24:26Z

Thanks for the thorough round-2 review @awsarron — addressed all 8 threads in ab41dc2:

#	Thread	Status	Detail
1	`_safe_join` duplication	✅ Already fixed	Both `manager.py` and `download.py` import from `utils.safe_join` since `4f56a52`
2	Lazy import design smell	✅ Fixed in `ab41dc2`	Replaced function-level lazy import with module-level `try/except` + sentinel
3	Unused SimWorld fields	✅ Already addressed	`_recording`/`_trajectory`/`_dataset_recorder` documented as living inside `_backend_state` dict (not separate fields). `_checkpoints` kept as top-level per @yinsong1986 to avoid monkey-patching on `reset()`
4	Alias shadowing bug	✅ Already fixed	`register_backend` checks `name` against `_BUILTIN_ALIASES` + `_runtime_aliases` since `4f56a52`, with test `test_register_rejects_builtin_alias_as_name`
5	`trossen_wxai` lost `robot_descriptions_module`	✅ Intentional	`widow_mj_description` → `trossen_wx250s/wx250s.xml` (single-arm WidowX 250s), wrong robot for the bimanual WXAI. `auto_download: false` is correct
6	`xmls/` path anomaly	✅ Expected	Matches upstream repo layout. `aliengo` (`xml/`), `jvrc` (`xml/`), `reachy_mini` (`mjcf/`) also use subdirectory paths
7	Repeated test Dummy class	✅ Already fixed	Refactored to `_make_dummy_engine_class()` factory + `dummy_engine_class` fixture — only 1 `class Dummy` remains
8	`_has_meshes` perf	✅ Already fixed	Cached with `_MESH_CACHE` + `os.scandir` early-exit since `77296d2`

Verified against the code, only the findings that hold up on re-read: simulation/factory.py - _import_backend_class: raise a descriptive ImportError when a built-in backend module is declared but not installed, instead of letting the raw ModuleNotFoundError surface to users. The default "mujoco" path now points at an unimplemented module, so create_simulation() would previously crash with a cryptic trace — now the error names the backend, suggests the extra to install, and points at register_backend(). simulation/model_registry.py - Drop the import-time _URDF_SEARCH_PATHS constant (snapshotted Path.cwd() at import, broke pytest/notebook chdir flows). Use utils.get_search_paths() at every lookup — same function already used by assets.manager, so we dedupe too. - Demote the module-level 'Asset manager available' log from INFO at import to a lazy DEBUG on first resolution. assets/download.py - _copy_and_clean: filter ignored patterns at copytree() time instead of deleting from the destination afterwards. Previous behaviour would clobber a user's own README/LICENSE/images in their asset cache. assets/manager.py - Gate _user_asset_path resolution through safe_join() so a malicious or corrupted user_robots.json cannot sneak '..' into model_xml and escape the asset cache. Matches the trust boundary of the built-in registry path. registry/user_registry.py - register_robot() now fails closed when the asset directory does not exist (previously silently accepted invalid registrations). - Warn on alias collisions at registration time (shadows-canonical, shadows-existing-alias) instead of letting resolution order silently decide the winner. tests/test_registry_integrity.py - test_aliases_unique_across_registry - test_no_alias_shadows_canonical_name - test_hardware_only_robots_declare_lerobot_type tests/test_simulation_factory.py (new) - test_default_backend_missing_raises_import_error_with_guidance (regression test for the cryptic-ModuleNotFoundError fix) - test_register_backend_loader_must_be_callable - test_register_backend_rejects_duplicate_without_force .gitignore - Ignore .ideation/ (local scratchpad for idea cycles). All 338 unit tests pass. ruff + mypy clean.

- assets/manager.py: remove 'import os as _os' alias — just import os at top - simulation/models.py: clarify _model vs _data vs _backend_state roles in docstring so backend implementers know which to use (each has a distinct purpose: core model handle, core state handle, catch-all dict) - utils.py: decouple get_base_dir() from STRANDS_ASSETS_DIR. Introduce STRANDS_BASE_DIR as the explicit override for the base directory so user_robots.json no longer lands in an unexpected parent of the assets path. STRANDS_ASSETS_DIR now *only* moves the assets subtree. - registry/user_registry.py: update module docstring to reflect the new STRANDS_BASE_DIR semantics - tests/test_user_registry.py: update fixture + integration tests to set STRANDS_BASE_DIR and STRANDS_ASSETS_DIR independently; add tests asserting that STRANDS_ASSETS_DIR does *not* move the registry / base dir

cagataycali · 2026-04-22T19:05:21Z

Thanks for the thorough review round @awsarron 🙏 — I've captured the 5 remaining items as a follow-up issue so they don't get lost, and going to merge this one so it stops being a rebase target for #85 and #86.

Follow-up: #105 (added to the project board)

Items tracked:

simulation/models.py — fold the _model / _data / _backend_state distinction into the class docstring more prominently
registry/robots.json — add an integration test that walks the registry and asserts every entry resolves, so upstream layout drifts fail CI immediately
assets/manager.py — add a lightweight is_robot_asset_present() so list_available_robots() / status queries don't trigger auto-download per entry
assets/download.py — fix stale [mujoco] → [sim] docstring reference
utils.py — remove the dead dedup check in get_search_paths()

All polish / minor correctness — none of them block the foundation landing. Will pick them up in a small follow-up PR before starting the MuJoCo backend rebase. 👍

🤖 AI agent response. Strands Agents. Feedback welcome!

@awsarron

Stale — all threads from this review have been resolved and addressed in subsequent commits. Superseded by @awsarron's approval on 2026-04-22.

Post-strands-labs#84 merge: SimWorld no longer carries MuJoCo-specific private fields (_xml, _robot_base_xml, _recording, _trajectory, _dataset_recorder, _tmpdir, _push_to_hub). These are MuJoCo backend implementation details and now live in world._backend_state, as the SimWorld docstring requests (prefer _backend_state over new fields). Migrated call sites: - mjcf_builder.py: tmpdir - policy_runner.py: recording, trajectory, dataset_recorder - recording.py: recording, trajectory, dataset_recorder, push_to_hub - scene_ops.py: robot_base_xml - simulation.py: xml, robot_base_xml, recording, trajectory Reads use dict[] where preceded by a guard that guarantees initialization (e.g. start_recording() sets before policy_runner reads), and .get() with sensible defaults where the key may be unset. Tests: 392 passed, 2 skipped (5 pre-existing test_path_validation failures are on main too — unrelated). Lint: ruff + mypy clean on 75 source files.

… registration Add Newton GPU simulation backend stub that implements SimEngine ABC. No GPU dependencies at import time — warp/newton are lazy-loaded. New files: - simulation/newton/__init__.py — lazy loading, no warp import - simulation/newton/config.py — NewtonConfig dataclass (7 solvers) - simulation/newton/simulation.py — NewtonSimulation(SimEngine) skeleton Modified files: - simulation/factory.py — register newton + aliases (warp, wp) - pyproject.toml — add [newton] extras (warp-lang, newton-sim) All 12 required SimEngine methods stubbed with NotImplementedError. Newton-specific extensions (replicate, diffsim, IK, cloth, etc.) stubbed. Factory kwargs flow through to NewtonConfig. Tests: 81 passed (config validation, factory round-trip, lazy import, ABC conformance, all stub methods verified). Part 1 of 7-PR Newton backend series (issue #314). Depends on PR strands-labs#84 (SimEngine ABC).

…spec dispatcher Bug: Simulation._dispatch_action filtered kwargs through a hardcoded whitelist that omitted observation_mapping, action_mapping, data_config, host, port, api_token, trust_remote_code, actions_per_step, use_processor, processor_overrides, and any other policy-specific kwarg. Agents could not wire a policy (GR00T, SmolVLA, lerobot_local) to a simulated robot through the AgentTool interface — sim joint names and canonical model keys never got reconciled, breaking sim↔real transfer. Fix: - simulation.py::_dispatch_action — replace the whitelist with a mapping-aware passthrough: for methods that declare **policy_kwargs, forward every input field that isn't already matched to a named parameter. Actions without **kwargs stay strict. - tool_spec.json — advertise observation_mapping, action_mapping, host, port, api_token, trust_remote_code, actions_per_step, use_processor, processor_overrides, device so agents can discover and use them. - tests/test_tool_spec_dispatch_policy_kwargs.py — 5 regression tests pinning the forwarding for run_policy / eval_policy / start_policy and verifying non-policy actions stay strict. End-to-end validation (MacBook Pro M-series, MPS): - create_world → add_robot(so100) → add_object(red_cube) → add_camera(camera1) → add_camera(camera2) → run_policy(policy_provider='lerobot_local', pretrained_name_or_path='lerobot/smolvla_base', device='mps', observation_mapping={'camera1': 'observation.images.camera1', 'camera2': 'observation.images.camera2', 'joint_position': 'observation.state'}, action_mapping={'action': 'joint_position'}) - SmolVLA downloaded, loaded on MPS, produced actions, sim stepped. 2 control steps / 25.4s wall → proves the full chain works. Quality: - ruff + mypy: clean (77 files) - hatch run test: 5/5 new tests pass; only pre-existing test_path_validation failures remain (noted by author on strands-labs#84).

Post-strands-labs#84 merge: SimWorld no longer carries MuJoCo-specific private fields (_xml, _robot_base_xml, _recording, _trajectory, _dataset_recorder, _tmpdir, _push_to_hub). These are MuJoCo backend implementation details and now live in world._backend_state, as the SimWorld docstring requests (prefer _backend_state over new fields). Migrated call sites: - mjcf_builder.py: tmpdir - policy_runner.py: recording, trajectory, dataset_recorder - recording.py: recording, trajectory, dataset_recorder, push_to_hub - scene_ops.py: robot_base_xml - simulation.py: xml, robot_base_xml, recording, trajectory Reads use dict[] where preceded by a guard that guarantees initialization (e.g. start_recording() sets before policy_runner reads), and .get() with sensible defaults where the key may be unset. Tests: 392 passed, 2 skipped (5 pre-existing test_path_validation failures are on main too — unrelated). Lint: ruff + mypy clean on 75 source files.

…spec dispatcher Bug: Simulation._dispatch_action filtered kwargs through a hardcoded whitelist that omitted observation_mapping, action_mapping, data_config, host, port, api_token, trust_remote_code, actions_per_step, use_processor, processor_overrides, and any other policy-specific kwarg. Agents could not wire a policy (GR00T, SmolVLA, lerobot_local) to a simulated robot through the AgentTool interface — sim joint names and canonical model keys never got reconciled, breaking sim↔real transfer. Fix: - simulation.py::_dispatch_action — replace the whitelist with a mapping-aware passthrough: for methods that declare **policy_kwargs, forward every input field that isn't already matched to a named parameter. Actions without **kwargs stay strict. - tool_spec.json — advertise observation_mapping, action_mapping, host, port, api_token, trust_remote_code, actions_per_step, use_processor, processor_overrides, device so agents can discover and use them. - tests/test_tool_spec_dispatch_policy_kwargs.py — 5 regression tests pinning the forwarding for run_policy / eval_policy / start_policy and verifying non-policy actions stay strict. End-to-end validation (MacBook Pro M-series, MPS): - create_world → add_robot(so100) → add_object(red_cube) → add_camera(camera1) → add_camera(camera2) → run_policy(policy_provider='lerobot_local', pretrained_name_or_path='lerobot/smolvla_base', device='mps', observation_mapping={'camera1': 'observation.images.camera1', 'camera2': 'observation.images.camera2', 'joint_position': 'observation.state'}, action_mapping={'action': 'joint_position'}) - SmolVLA downloaded, loaded on MPS, produced actions, sim stepped. 2 control steps / 25.4s wall → proves the full chain works. Quality: - ruff + mypy: clean (77 files) - hatch run test: 5/5 new tests pass; only pre-existing test_path_validation failures remain (noted by author on strands-labs#84).

cagataycali added this to Strands Labs - Robots Apr 1, 2026

github-project-automation Bot moved this to Backlog in Strands Labs - Robots Apr 1, 2026

cagataycali commented Apr 1, 2026

View reviewed changes

Comment thread .github/workflows/test-lint.yml Outdated

cagataycali mentioned this pull request Apr 1, 2026

feat: MuJoCo simulation backend - AgentTool with 35 actions #85

Open

cagataycali moved this from Backlog to In review in Strands Labs - Robots Apr 1, 2026

This was referenced Apr 1, 2026

docs: rewrite README, update AGENTS.md, add 8 examples #87

Open

early review: feat: MuJoCo Simulation Backend + Build System Modernization #80

Closed

yinsong1986 reviewed Apr 1, 2026

View reviewed changes

Comment thread strands_robots/simulation/models.py Outdated

Comment thread strands_robots/simulation/base.py

Comment thread strands_robots/simulation/__init__.py Outdated

cagataycali force-pushed the feat/simulation-foundation branch from e5d70ca to 6bb80bd Compare April 1, 2026 20:03

yinsong1986 approved these changes Apr 1, 2026

View reviewed changes

awsarron previously requested changes Apr 3, 2026

View reviewed changes

awsarron reviewed Apr 3, 2026

View reviewed changes

Comment thread pyproject.toml

awsarron reviewed Apr 3, 2026

View reviewed changes

Comment thread tests/test_simulation_foundation.py Outdated

cagataycali force-pushed the feat/simulation-foundation branch from 2594b1e to 2217260 Compare April 3, 2026 20:54

strands-labs deleted a comment from shipitfast Apr 3, 2026

cagataycali moved this from In review to In progress in Strands Labs - Robots Apr 5, 2026

cagataycali moved this from In progress to In review in Strands Labs - Robots Apr 5, 2026

cagataycali requested a review from awsarron April 6, 2026 05:23

cagataycali added this to the v0.4 milestone Apr 6, 2026

awsarron previously requested changes Apr 10, 2026

View reviewed changes

awsarron reviewed Apr 10, 2026

View reviewed changes

Comment thread strands_robots/tools/download_assets.py Outdated

awsarron reviewed Apr 10, 2026

View reviewed changes

Comment thread strands_robots/registry/robots.json Outdated

awsarron reviewed Apr 10, 2026

View reviewed changes

Comment thread strands_robots/simulation/models.py Outdated

cagataycali requested a review from awsarron April 22, 2026 01:52

awsarron previously approved these changes Apr 22, 2026

View reviewed changes

Comment thread strands_robots/utils.py

Comment thread strands_robots/assets/manager.py Outdated

Comment thread strands_robots/simulation/models.py

Comment thread strands_robots/registry/robots.json

cagataycali dismissed awsarron’s stale review via a2be111 April 22, 2026 16:51

cagataycali force-pushed the feat/simulation-foundation branch from a2be111 to 3a5c14b Compare April 22, 2026 16:52

cagataycali force-pushed the feat/simulation-foundation branch from 3a5c14b to fc89b1f Compare April 22, 2026 16:54

cagataycali requested a review from awsarron April 22, 2026 17:00

cagataycali enabled auto-merge (squash) April 22, 2026 17:01

cagataycali disabled auto-merge April 22, 2026 17:01

cagataycali enabled auto-merge (squash) April 22, 2026 17:03

cagataycali disabled auto-merge April 22, 2026 17:06

cagataycali enabled auto-merge (squash) April 22, 2026 17:09

awsarron approved these changes Apr 22, 2026

View reviewed changes

Comment thread strands_robots/assets/manager.py

Comment thread strands_robots/simulation/models.py

Comment thread strands_robots/assets/download.py

Comment thread strands_robots/utils.py

cagataycali mentioned this pull request Apr 22, 2026

Follow-up: address review items from PR #84 #105

Closed

3 tasks

cagataycali merged commit 6d80fcc into strands-labs:main Apr 22, 2026
2 checks passed

github-project-automation Bot moved this from In review to Done in Strands Labs - Robots Apr 22, 2026

cagataycali mentioned this pull request Apr 24, 2026

fix: address PR #84 follow-up items (registry tests, asset presence check, dead code removal) #106

Merged

cagataycali added a commit that referenced this pull request Apr 27, 2026

fix: address PR #84 follow-up items (#105) (#106)

7228af3

Conversation

cagataycali commented Apr 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

TL;DR

What changed

Simulation abstractions (strands_robots/simulation/)

Assets (strands_robots/assets/)

Tools (strands_robots/tools/)

Registry (strands_robots/registry/)

Other

Design decisions

SimEngine ABC

Asset resolution order

Security

Testing

Review history

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

yinsong1986 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

cagataycali commented Apr 6, 2026

Uh oh!

awsarron left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

cagataycali commented Apr 22, 2026

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

cagataycali commented Apr 22, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

cagataycali commented Apr 1, 2026 •

edited

Loading

Simulation abstractions (`strands_robots/simulation/`)

Assets (`strands_robots/assets/`)

Tools (`strands_robots/tools/`)

Registry (`strands_robots/registry/`)

awsarron left a comment •

edited

Loading